Add option to list plugins #2319
Open
+160
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skoeva
force-pushed
the
list-plugins-option
branch
from
dff3d5c
to
3a3ba2b
Compare
|
Backend Code coverage changed from 59.3% to 59.3%. Change: 0% 😃. |
illume
reviewed
Sep 11, 2024
There was a problem hiding this comment.
Thanks.
I wrote a few notes about how the backend/ code is organized.
btw, Probably this option could be documented somewhere? We don't have a page for all options. The headlamp-plugin tool has them listed in its README.md. If you don't have any inspiration for this at the moment, we can document the command line arguments in a separate PR.
skoeva
force-pushed
the
list-plugins-option
branch
2 times, most recently
from
5d19c8a
to
2c966db
Compare
|
Backend Code coverage changed from 60.1% to 60.0%. Change: -.1% 😞. |
skoeva
force-pushed
the
list-plugins-option
branch
from
2c966db
to
fe72ef2
Compare
|
Backend Code coverage changed from 60.1% to 60.0%. Change: -.1% 😞. |
skoeva
force-pushed
the
list-plugins-option
branch
from
fe72ef2
to
e1efd3d
Compare
|
Backend Code coverage changed from 60.1% to 60.0%. Change: -.1% 😞. |
|
Looks good to me. But I think we can wait for Joaquim to see if it meets his requirements before merging. ps. To make the code coverage bot use a happy face emoji you might consider adding a test for ListPlugins(possibly using a mock for GenerateSeparatePluginPaths). |
skoeva
force-pushed
the
list-plugins-option
branch
2 times, most recently
from
05b326d
to
8e99e17
Compare
|
was looking into this from our sync earlier this week so far so good |
|
Backend Code coverage changed from 60.1% to 60.4%. Change: .3% 😃. |
joaquimrocha
requested changes
Sep 19, 2024
app/electron/main.ts
Outdated
| // If the user wants to list plugins, we don't need to start the app. | ||
| if (listPlugins) { | ||
| const staticDir = path.join(process.resourcesPath, '.plugins'); | ||
| const pluginsDir = path.join(app.getPath('userData'), 'plugins'); |
There was a problem hiding this comment.
Is there a way to get this one from the plugin manager? We should always try to have one source of truth for data.
There was a problem hiding this comment.
Tried testing using PluginManager's list function, but it would break and return empty lists. I tried testing changes to checkValidPluginFolder to see if that would fix it but it caused new JS issues. I've made the other change and can continue testing with PluginManager but I seem to be hitting a wall with this
There was a problem hiding this comment.
I think we are using 3 sources of truth in this PR:
- The backend
- The PluginManager
- This app module
If we want to keep the backend and plugin manager independent, I think we need ot keep those two separated. But this main module needs to be using the source from one of the others. Since the backend has a new list-plugins command, we could as well just call that from command from here and not replicate the logic we got there.
There was a problem hiding this comment.
Changed this to call execSync using the backend logic
skoeva
force-pushed
the
list-plugins-option
branch
from
8e99e17
to
cd07a56
Compare
|
Backend Code coverage changed from 60.1% to 60.4%. Change: .3% 😃. |
skoeva
force-pushed
the
list-plugins-option
branch
2 times, most recently
from
0834cd7
to
11cde63
Compare
|
Backend Code coverage changed from 60.1% to 60.3%. Change: .2% 😃. |
joaquimrocha
requested changes
Sep 25, 2024
| @@ -25,6 +25,7 @@ type Config struct { | |||
| InsecureSsl bool `koanf:"insecure-ssl"` | |||
| EnableHelm bool `koanf:"enable-helm"` | |||
| EnableDynamicClusters bool `koanf:"enable-dynamic-clusters"` | |||
| ListPlugins bool `koanf:"list-plugins"` | |||
There was a problem hiding this comment.
@yolossn This is not really a config option. It's rather a subcommand.
So I wonder if we shouldn't include it in the koanf config. Do you know if there's an option to modify that in koanf, or should we use the flag parsing in a different way?
app/electron/main.ts
Outdated
| // If the user wants to list plugins, we don't need to start the app. | ||
| if (listPlugins) { | ||
| const staticDir = path.join(process.resourcesPath, '.plugins'); | ||
| const pluginsDir = path.join(app.getPath('userData'), 'plugins'); |
There was a problem hiding this comment.
I think we are using 3 sources of truth in this PR:
- The backend
- The PluginManager
- This app module
If we want to keep the backend and plugin manager independent, I think we need ot keep those two separated. But this main module needs to be using the source from one of the others. Since the backend has a new list-plugins command, we could as well just call that from command from here and not replicate the logic we got there.
skoeva
force-pushed
the
list-plugins-option
branch
from
11cde63
to
01cb826
Compare
dosubot
bot
added
the
size:L
This change creates a CLI option in the backend + desktop to list all plugins, with a distinction between static/shipped plugins and user-added ones. Fixes: #2161 Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
skoeva
force-pushed
the
list-plugins-option
branch
from
01cb826
to
a11ec3a
Compare
|
Backend Code coverage changed from 60.1% to 60.4%. Change: .3% 😃. Coverage report |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change creates a CLI option in the backend + desktop to list all plugins, with a distinction between static/shipped plugins and user-added ones.
Fixes: #2161
Testing
Backend
make backendcd backend && ./headlamp-server --list-pluginsand ensure that the list of plugins shows upDesktop
cd app && npm run buildcd dist/linux-unpacked && ./headlamp --list-pluginsand ensure that the list of plugins shows up